Skip to content

Fix: Redundant account destruction after state modification #1355

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 5 commits into
base: forks/osaka
Choose a base branch
from
Open

Fix: Redundant account destruction after state modification #1355

wants to merge 5 commits into from

Conversation

EperezOk
Copy link

What was wrong?

Related to Issue #1354

How was it fixed?

Removed redundant checks and account destruction operations.

Cute Animal Picture

Jiraffe

@EperezOk EperezOk changed the title fix: redundant account destruction after state modification Fix: Redundant account destruction after state modification Jul 29, 2025
@SamWilsn SamWilsn force-pushed the fix/redundant-account-existence-checks branch from 3adbb4e to a9ce06c Compare July 31, 2025 16:20
@SamWilsn SamWilsn changed the base branch from master to forks/osaka July 31, 2025 16:20
@EperezOk
Copy link
Author

EperezOk commented Aug 1, 2025

@SamWilsn please let me know if anything else is needed from my side or if I should mark the PR as "ready for review"

@SamWilsn
Copy link
Contributor

Hey! Thanks for the fix.

If you're feeling lazy, you can leave this and we'll do the rest of the work ourselves eventually. Otherwise, it'll need to be ported to the other post-merge forks before we can merge.

If you're feeling super enthusiastic, might be worth investigating whether we can remove account_exists_and_is_empty entirely from post-merge forks.

@EperezOk
Copy link
Author

Hi @SamWilsn!

I ported the change to the other post-merge forks. I also confirmed that account_exists_and_is_empty is no longer needed in those forks, so I removed the function and inlined it within modify_state.

@EperezOk EperezOk marked this pull request as ready for review August 31, 2025 06:05
SamWilsn
SamWilsn approved these changes Sep 2, 2025
View reviewed changes
Copy link
Contributor

@SamWilsn SamWilsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense as far as I can tell :3

@codecov-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 93.78830% with 223 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.99%. Comparing base (301eb89) to head (07d945d).
⚠️ Report is 2186 commits behind head on forks/osaka.

Files with missing lines Patch % Lines
src/ethereum/osaka/fork.py 78.89% 33 Missing and 28 partials ⚠️
...hereum/osaka/vm/precompiled_contracts/alt_bn128.py 53.76% 37 Missing and 6 partials ⚠️
src/ethereum/osaka/transactions.py 92.27% 10 Missing and 11 partials ⚠️
src/ethereum/osaka/vm/instructions/arithmetic.py 85.58% 8 Missing and 8 partials ⚠️
...aka/vm/precompiled_contracts/bls12_381/__init__.py 82.41% 8 Missing and 8 partials ⚠️
src/ethereum/osaka/vm/instructions/system.py 94.57% 8 Missing and 4 partials ⚠️
...hereum/osaka/vm/precompiled_contracts/ecrecover.py 58.62% 11 Missing and 1 partial ⚠️
src/ethereum/osaka/trie.py 94.40% 4 Missing and 4 partials ⚠️
src/ethereum/osaka/vm/interpreter.py 95.16% 3 Missing and 3 partials ⚠️
src/ethereum/osaka/vm/instructions/stack.py 95.95% 2 Missing and 2 partials ⚠️
... and 12 more
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
Additional details and impacted files 
@@               Coverage Diff               @@
##           forks/osaka    #1355      +/-   ##
===============================================
+ Coverage        94.16%   94.99%   +0.83%     
===============================================
  Files              583      583              
  Lines            34666    34637      -29     
  Branches          3073     3056      -17     
===============================================
+ Hits             32643    32904     +261     
+ Misses            1487     1189     -298     
- Partials           536      544       +8
Flag Coverage Δ
unittests 94.99% <93.78%> (+0.83%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SamWilsn SamWilsn SamWilsn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@EperezOk @SamWilsn @codecov-commenter